[Core feature] Flytekit should support `unsafe` mode for types #2419

Mecoli1219 · 2024-05-15T05:52:56Z

Tracking issue

Why are the changes needed?

Use Case: When the user wants to apply Flytekit to their Python codebase, but annotating the type is too hard or time-consuming, we should support unsafe mode for easy usage.

What changes were proposed in this pull request?

I add unsafe kwarg to both task and workflow, and the default value of unsafe is False
If the unsafe kwarg is True and the types of inputs or outputs are not specified (which is None), replace the type from None to Optional[Any]. This will trigger FlytePickleTransformer to handle each unspecified type.

How was this patch tested?

Two tests are created to examine the ability of unsafe kwarg.

test_unsafe_input_wf_and_task: This test mainly examines the unspecified input types. If the unsafe is False, the workflow or task should throw errors. Otherwise, it should succeed.
test_unsafe_wf_and_task: This test mimics the use case mentioned above, where the user might not specify all the types in their codebase. The workflow should succeed if the unsafe kwarg of all tasks and workflow are set to True.

Setup process

Screenshots

Check all the applicable boxes

I updated the documentation accordingly.
All new and existing tests passed.
All commits are signed-off.

Related PRs

flyteorg/flyte#5319

Docs link

Signed-off-by: Mecoli1219 <[email protected]>

pingsutw · 2024-05-16T11:21:05Z

tests/flytekit/unit/core/test_type_hints.py

+        return t1(a=a)
+
+    @workflow
+    def wf1_wo_unsafe2(a: int) -> int:


Did you test this workflow in the sandbox cluster?

No, I only test it on my local environment. Can I ask how to try it?

This PR can help you, you can either use imageSpec or Dockerfile to create your own image.
#1870

from flytekit import task, Secret, workflow, ImageSpec flytekit_dev_version = "https://github.com/Mecoli1219/flytekit.git@c7c9a2ad2bdf6799ec324955a023281cca030038" image = ImageSpec( registry="localhost:30000", platform="linux/amd64", apt_packages=["git"], packages=[ f"git+{flytekit_dev_version}", ], ) @task(unsafe=True, container_image=image) def t1(a) -> int: if type(a) == int: return a + 1 return 0 @workflow(unsafe=True) def wf1_with_unsafe(a) -> int: return t1(a=a)

The above code will fail. The reason is detailed in flyteorg/flyte#5261.

from flytekit import task, Secret, workflow, ImageSpec from typing import Tuple flytekit_dev_version = "https://github.com/Mecoli1219/flytekit.git@9885189b2af95c0c0dfa94ff92b71e865d95cf80" image = ImageSpec( registry="localhost:30000", platform="linux/amd64", builder="fast-builder", apt_packages=["git"], packages=[ f"git+{flytekit_dev_version}", ], ) @task(unsafe=True, container_image=image) def t1(a) -> int: if type(a) == int: return a + 1 return 0 @workflow def wf1_with_unsafe() -> Tuple[int, int, int]: a1 = t1(a=1) a2 = t1(a="1") a3 = t1(a=None) #! This will fail return a1, a2, a3

Currently, the input with a value None will fail, but the issue is solved with this PR (flyteorg/flyte#5408).

Signed-off-by: Mecoli1219 <[email protected]>

Mecoli1219 · 2024-06-19T14:33:27Z

@pingsutw I created 2 new unit tests, and both of them passed.

Signed-off-by: Kevin Su <[email protected]>

codecov · 2024-06-21T22:43:45Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.99%. Comparing base (4bef161) to head (7db0869).
Report is 5 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff             @@
##           master    #2419       +/-   ##
===========================================
+ Coverage   46.48%   74.99%   +28.51%     
===========================================
  Files         199      279       +80     
  Lines       20707    24265     +3558     
  Branches     2665     2679       +14     
===========================================
+ Hits         9625    18197     +8572     
+ Misses      10610     5341     -5269     
- Partials      472      727      +255

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Mecoli1219 <[email protected]>

thomasjpfan · 2024-09-28T18:45:05Z

flytekit/core/task.py

@@ -130,6 +130,7 @@ def task(
    pod_template: Optional["PodTemplate"] = ...,
    pod_template_name: Optional[str] = ...,
    accelerator: Optional[BaseAccelerator] = ...,
+    unsafe: bool = ...,


I prefer to have a more explicit name. Something like:

Suggested change

unsafe: bool = ...,

pickle_untyped: bool = ...,

Signed-off-by: Mecoli1219 <[email protected]>

thomasjpfan · 2024-11-05T18:46:25Z

flytekit/core/python_function_task.py

@@ -112,6 +112,7 @@ def __init__(
        node_dependency_hints: Optional[
            Iterable[Union["PythonFunctionTask", "_annotated_launch_plan.LaunchPlan", WorkflowBase]]
        ] = None,
+        pickle_untyped: bool = False,


@kumare3 Since you proposed the safe parameter name, are you okay with using pickle_untyped?

My concerned is that safe=False could mean so many unsafe behavior. pickle_untyped is explicit about the behavior.

thomasjpfan · 2024-11-05T18:47:02Z

flytekit/types/pickle/pickle.py

        uri = lv.scalar.blob.uri
        return FlytePickle.from_pickle(uri)

    def to_literal(self, ctx: FlyteContext, python_val: T, python_type: Type[T], expected: LiteralType) -> Literal:
        if python_val is None:
-            raise AssertionError("Cannot pickle None Value.")
+            # raise AssertionError("Cannot pickle None Value.")


Can this be removed?

yes sure. I'll remove that recently

Signed-off-by: Mecoli1219 <[email protected]>

Mecoli1219 · 2024-11-06T04:04:52Z

NVM, it is still the same problem as mentioned in the comment above 6 months ago.

@task(pickle_untyped=True, container_image=image)
def t1(a) -> int:
    if type(a) == int:
        return a + 1
    return 0

@workflow
def wf1_with_unsafe() -> Tuple[int, int, int]:
    a1 = t1(a=1)
    a2 = t1(a="1")
    a3 = t1(a=None)    #! This will fail
    return a1, a2, a3

Currently, the input with a value None will fail

Mecoli1219 · 2024-11-06T04:06:06Z

First example

@task(pickle_untyped=True, container_image=image)
def t1(a) -> int:
    if type(a) == int:
        return a + 1
    return 0


@workflow
def wf1_with_unsafe() -> Tuple[int, int, int]:
    a1 = t1(a=1)
    a2 = t1(a="1")
    # a3 = t1(a=None)  #! This will fail
    return a1, a2, a2

Second example

@task(pickle_untyped=True, container_image=image)
def t1(a):
    if type(a) == int:
        return a + 1
    return 1


@workflow
def wf1_with_unsafe() -> Tuple[Any, Any, Any]:
    a1 = t1(a=1)
    a2 = t1(a="1")
    # a3 = t1(a=None)  #! This will fail
    return a1, a2, a2

Signed-off-by: Mecoli1219 <[email protected]>

Mecoli1219 · 2024-11-06T04:40:51Z

I revert the support for None type in pickle. I think we could discuss that later and solve that in another PR if finally decided to do that.

Mecoli1219 added 3 commits May 15, 2024 13:32

Enable unsafe type task & workflow

3570363

Signed-off-by: Mecoli1219 <[email protected]>

Add unsafe unit test

8c7e5fa

Signed-off-by: Mecoli1219 <[email protected]>

Formatting

c7c9a2a

Signed-off-by: Mecoli1219 <[email protected]>

Mecoli1219 requested review from wild-endeavor, kumare3, eapolinario, pingsutw, cosmicBboy and samhita-alla as code owners May 15, 2024 05:52

pingsutw reviewed May 16, 2024

View reviewed changes

Mecoli1219 added 4 commits May 29, 2024 13:48

Change Optional[Any] to Any

1524c81

Signed-off-by: Mecoli1219 <[email protected]>

Fix Scalar Void() error

0519fe6

Signed-off-by: Mecoli1219 <[email protected]>

Modify unit test

9885189

Signed-off-by: Mecoli1219 <[email protected]>

Update 2 unit tests

f107b35

Signed-off-by: Mecoli1219 <[email protected]>

pingsutw added 2 commits June 21, 2024 15:33

merged master

8db4bce

Signed-off-by: Kevin Su <[email protected]>

nit

aa1760c

Signed-off-by: Kevin Su <[email protected]>

pingsutw previously approved these changes Jun 22, 2024

View reviewed changes

Merge branch 'master' into #5319

df0f898

Mecoli1219 dismissed pingsutw’s stale review via df0f898 September 27, 2024 06:15

Mecoli1219 requested a review from thomasjpfan as a code owner September 27, 2024 06:15

Fix merge

8368723

Signed-off-by: Mecoli1219 <[email protected]>

thomasjpfan reviewed Sep 28, 2024

View reviewed changes

Mecoli1219 and others added 4 commits September 28, 2024 16:29

Merge remote-tracking branch 'upstream/master' into #5319

1b5bfeb

Signed-off-by: Mecoli1219 <[email protected]>

rename unsafe to pickle_untyped

51ea34c

Signed-off-by: Mecoli1219 <[email protected]>

Merge branch 'master' into #5319

d35c231

Fix merge

933458a

Signed-off-by: Mecoli1219 <[email protected]>

Future-Outlier assigned Mecoli1219 and pingsutw Oct 25, 2024

thomasjpfan reviewed Nov 5, 2024

View reviewed changes

Mecoli1219 added 3 commits November 5, 2024 19:40

nit

11b5d9c

Signed-off-by: Mecoli1219 <[email protected]>

merge upstream

75296df

Signed-off-by: Mecoli1219 <[email protected]>

Update unit test

7db0869

Signed-off-by: Mecoli1219 <[email protected]>

Mecoli1219 changed the title ~~[Core feature] Flytekit should support unsafe mode for types~~ [WIP] [Core feature] Flytekit should support unsafe mode for types Nov 6, 2024

Mecoli1219 changed the title ~~[WIP] [Core feature] Flytekit should support unsafe mode for types~~ [Core feature] Flytekit should support unsafe mode for types Nov 6, 2024

revert None support

43f8948

Signed-off-by: Mecoli1219 <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Core feature] Flytekit should support `unsafe` mode for types #2419

[Core feature] Flytekit should support `unsafe` mode for types #2419

Mecoli1219 commented May 15, 2024 •

edited

Loading

pingsutw May 16, 2024

Mecoli1219 May 16, 2024

Future-Outlier May 17, 2024 •

edited

Loading

Mecoli1219 May 29, 2024

Mecoli1219 May 29, 2024

Mecoli1219 May 29, 2024 •

edited

Loading

Mecoli1219 commented Jun 19, 2024

codecov bot commented Jun 21, 2024 •

edited

Loading

thomasjpfan Sep 28, 2024

thomasjpfan Nov 5, 2024 •

edited

Loading

thomasjpfan Nov 5, 2024

Mecoli1219 Nov 5, 2024

Mecoli1219 commented Nov 6, 2024 •

edited

Loading

Mecoli1219 commented Nov 6, 2024 •

edited

Loading

Mecoli1219 commented Nov 6, 2024

[Core feature] Flytekit should support unsafe mode for types #2419

Are you sure you want to change the base?

[Core feature] Flytekit should support unsafe mode for types #2419

Conversation

Mecoli1219 commented May 15, 2024 • edited Loading

Tracking issue

Why are the changes needed?

What changes were proposed in this pull request?

How was this patch tested?

Setup process

Screenshots

Check all the applicable boxes

Related PRs

Docs link

pingsutw May 16, 2024

Choose a reason for hiding this comment

Mecoli1219 May 16, 2024

Choose a reason for hiding this comment

Future-Outlier May 17, 2024 • edited Loading

Choose a reason for hiding this comment

Mecoli1219 May 29, 2024

Choose a reason for hiding this comment

Mecoli1219 May 29, 2024

Choose a reason for hiding this comment

Mecoli1219 May 29, 2024 • edited Loading

Choose a reason for hiding this comment

Mecoli1219 commented Jun 19, 2024

codecov bot commented Jun 21, 2024 • edited Loading

Codecov Report

thomasjpfan Sep 28, 2024

Choose a reason for hiding this comment

thomasjpfan Nov 5, 2024 • edited Loading

Choose a reason for hiding this comment

thomasjpfan Nov 5, 2024

Choose a reason for hiding this comment

Mecoli1219 Nov 5, 2024

Choose a reason for hiding this comment

Mecoli1219 commented Nov 6, 2024 • edited Loading

Mecoli1219 commented Nov 6, 2024 • edited Loading

Mecoli1219 commented Nov 6, 2024

[Core feature] Flytekit should support `unsafe` mode for types #2419

[Core feature] Flytekit should support `unsafe` mode for types #2419

Mecoli1219 commented May 15, 2024 •

edited

Loading

Future-Outlier May 17, 2024 •

edited

Loading

Mecoli1219 May 29, 2024 •

edited

Loading

codecov bot commented Jun 21, 2024 •

edited

Loading

thomasjpfan Nov 5, 2024 •

edited

Loading

Mecoli1219 commented Nov 6, 2024 •

edited

Loading

Mecoli1219 commented Nov 6, 2024 •

edited

Loading